WIP: Add email domain filter #98
Conversation
|
This makes sense. I'll take a look. |
|
@jenkinsci/code-reviewers Please check out this change. |
|
@jdmulloy mind if I make a feature request? A CSV list of domains in order of preference. Some companies have contractors who would be using contractor emails. If not, that's fine, I could try adding it later. |
|
@samrocketman I've added in some logging and added the csv feature. |
|
Thanks for humoring my request @jdmulloy. I'll test running and upgrading to your version to ensure stability. If all goes well I'll merge it. I think this is a pretty cool feature. |
| } | ||
|
|
||
| writer.startNode("forceGithubEmail"); | ||
| //TODO: Is there a better way to do this? |
There was a problem hiding this comment.
Is there a better/preferred way to store a boolean value in the config file? If not and what I have is ok I'll remove the TODO comment.
There was a problem hiding this comment.
Check how other plugins save and load booleans. I'll check as well.
|
Once it's approved I can squash the commits to make the history cleaner. |
|
I have not forgotten about this change. I was going to look at it over the weekend and did not get a chance. I'll see about looking at it during the evenings this week. |
| String oauthScopes) { | ||
| String oauthScopes, | ||
| String emailDomains, | ||
| Boolean forceGithubEmail) { |
There was a problem hiding this comment.
This will not cleanly migrate configuration. Please overload this method with another deprecated method so that configurations cleanly migrate.
There was a problem hiding this comment.
Revert the old failed test
I accidentally advised you to update a test rather than address its failure. A mistake on my part.
- Please revert the change so the tests fail again.
- Update your PR so the tests pass.
- Add a few additional tests to the existing tests so that your change is properly tested.
I can help you with this if you need it.
Fix configuration migration
This will not cleanly migrate configuration. Please overload GithubSecurityRealm() constructor with another deprecated constructor so that configurations cleanly migrate.
Note: this should fix existing failed tests.
A simple test:
- Build this plugin.
- Start Jenkins with the stable GitHub OAuth plugin configured.
- Upgrade to this built plugin and see how the configuration migration behaves.
I'll take a closer look at this during the week. There are other plugins which do as I describe. Try to find examples there. Otherwise, I'll try to find an example.
Example:
/*
This method is deprecated.
@deprecated use GithubSecurityRealm(githubWebUri, githubApiUri, clientID, clientSecret, oauthScopes, emailDomains, forceGithubEmail)
*/
@Deprecated
public GithubSecurityRealm(String githubWebUri,
String githubApiUri,
String clientID,
String clientSecret,
String oauthScopes) {
GithubSecurityRealm(githubWebUri, githubApiUri, clientID, clientSecret, oauthScopes, "", false);
}Fix object equality
^^ the above link uses an "equals()" method. You're adding new properties to the object without testing for equality. That means if there's differences in your settings, the equals() method will misreport that the two objects are equal in terms of property values.
|
@samrocketman I'll work on overloading the method and fixing the tests. How do I run the tests that are failing? When I build the plugin I don't get any test failures, so there is probably something I'm missing. I can't find any failing tests via this PR, but I may not know where to look. Thanks |
|
Actually I think I figured out what you were saying. You were talking about the tests that were failing until I changed them. Correct? |
|
@samrocketman I added the deprecated method, but it fails to compile with the following error. I can't figure out why it can't find the method. I've tried googling. |
| } | ||
|
|
||
| @Test | ||
| public void testHasScope_true() { | ||
| GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,user,user:email", "", false); | ||
| GithubSecurityRealm b = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,user,user:email"); | ||
| GithubSecurityRealm a = new GithubSecurityRealm("http:jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,user,user:email"); |
There was a problem hiding this comment.
Did you mean to remove // from http://? This comment applies to other parts as well.
There was a problem hiding this comment.
Thanks for catching that. It was an accident. I had commented out a bunch of lines while debugging and used a regex to remove the comments.
|
Let me know when your'e ready for me to review/test this change for merging. |
|
Please squash and rebase on master. |
jdmulloy
force-pushed
the
jdmulloy/add_email_domain_filter
branch
from
c23a1da to
793d598
Compare
|
@samrocketman I've squashed and rebased on master. I tested upgrading the plugin and it works now. I had to fix handling of the forceGithubEmail setting being missing in config.xml. The unmarshaling code was setting it to Null instead of False. The todo comment for handling the boolean forceGithubEmail is still there. The useSecurity setting in core uses "true" in config.xml to store true, but I couldn't find the relevant marshaling and unmarshaling code for that. So I couldn't figure out how that's done in core. |
|
Thanks I'll look it over and test it. |
|
Upgraded by uploading via the Click here for stack traceTo reproduce this issue I used the my jenkins-bootstrap-jervis project which you can use
|
|
However, when I restarted Jenkins the stacktrace went away. |
|
I'm going to investigate this more before merging. |
|
Also note, if you wanted to test this yourself without the complication of learning the scripts I publish packages of the version I tested. https://github.com/samrocketman/jenkins-bootstrap-jervis/releases/tag/jervis-bootstrap-2.89.4.3 |
|
@jdmulloy I just released a bunch of security fixes and a critical bug fix. Unfortunately, I didn't get a chance to test your change as part of the release. Would you mind merging in the latest master and resolving the merge conflicts? If not, then I can take a look and update it after I move. |
This is useful for operators who have users whose Github accounts have multiple email addresses and their default address is a personal address. The current implementation leads to Jenkins storing the default github address, which in some cases is a personal account. This change allows an operator to specify the domain of their org, and the plugin will look at all the emails for a user and if it finds one that matches the specified domain it will prefer that email over the primary. If the setting is empty it will behave as before.
I also added a checkbox to always override the stored value with what is found in Github.
This is very much a work in progress. I'm not an experienced Java developer and I did this in one working day. It probably also needs some tests for the new functionality. I'd appreciate feedback.